Skip to content

Add MCViNE sample kernels as Union processes (and standalone samples) - #2716

Merged
willend merged 7 commits into
mccode-dev:mainfrom
Fahima-Islam:mcvine-kernels
Oct 2, 2026
Merged

willend merged 7 commits into
mccode-dev:mainfrom
Fahima-Islam:mcvine-kernels

Conversation

@Fahima-Islam

Copy link
Copy Markdown
Contributor

Free-form text area

This PR adds 16 sample scattering kernels from MCViNE (https://github.com/mcvine/mcvine, mccomponents/lib/kernels/sample, plus SANS2D\_ongrid from https://github.com/mcvine/acc). McStas has no equivalent for any of them. Each kernel is available in two forms:

  • a Union process contrib/MCViNE\_<kernel>\_process.comp (geometry, containers, absorption and multiple scattering by Union), and
  • a standalone sample contrib/MCViNE\_<kernel>.comp (box / (hollow) cylinder / sphere, same transport as MCViNE's HomogeneousNeutronScatterer).

Both forms call the same kernel code in share/mcvine-lib.c.

Kernels added

Kernel Physics
ConstantEnergyTransfer, ConstantQE, ConstantvQE fixed ΔE / fixed (|Q|,E) / fixed Q vector (resolution and test kernels)
E_Q, Broadened_E_Q, LorentzianBroadened_E_Q isotropic dispersion E(|Q|) with S(Q), optionally Gaussian or Lorentzian broadened
E_vQ single-crystal dispersion E(Qx,Qy,Qz) with S(Qx,Qy,Qz)
SQ, SvQ elastic S(|Q|) or S(Q) (expression or grid)
SQE isotropic S(Q,E) from an expression or an Isotropic\_Sqw-format grid, optional final-energy focusing
DGSSXRes direct-geometry single-crystal resolution kernel
Phonon_IncoherentElastic, Phonon_IncoherentInelastic incoherent elastic with Debye–Waller factor; one-phonon incoherent scattering from a DOS (optional Ef focusing)
Phonon_CoherentInelastic_PolyXtal, Phonon_CoherentInelastic_SingleXtal coherent one-phonon scattering from a phonon dispersion + polarization vectors on a grid (MCViNE IDF files, e.g. from phonopy)
SANS2D_ongrid SANS from a 2D S(Qx,Qy) map

MCViNE takes functions such as E(Q) and S(Q) as strings evaluated at run time (fparser). The same is supported here through a small built-in expression evaluator, e.g. E\_Q="20\*sin(Q\*1.5)^2".

Kernels not added, because McStas already covers them: isotropic scattering (Incoherent), gridded S(Q,E) (Isotropic\_Sqw), powder and single-crystal diffraction (PowderN, Single\_crystal), SANS spheres, and multiphonon (IncoherentPhonon\_process, NCrystal). MCViNE's EPSC diffraction kernel is unfinished upstream and is not ported.

Runtime-library change (Union core, 12 added lines). Union dispatches physics with a switch on enum process. This PR registers one new process type, MCViNE:

  • share/union-lib.c: adds an enum entry and a data\_transfer\_union member
  • share/union-suffix.c: adds two case MCViNE: blocks guarded by PROCESS\_MCVINE\_DETECTOR, the same pattern as the other processes

All 16 kernels share this type. Its storage struct holds pointers to the kernel and to its sampling function, so further MCViNE kernels need no more core changes. Existing processes are untouched. The processes are NOACC (CPU only).

New files

  • share/mcvine-lib.h/.c – kernel physics, expression evaluator, table/grid readers (files found through Open\_File), DOS and Debye–Waller helpers, MCViNE IDF phonon readers, shapes and standalone transport
  • share/mcvine-union-lib.h/.c – Union glue
  • contrib/MCViNE\_\*\_process.comp (16), contrib/MCViNE\_\*.comp (16)
  • contrib/doc/MCViNE\_kernels.md – overview, input formats, differences from MCViNE, validation
  • examples/Tests\_samples/Test\_MCViNE\_\*/ (16 test instruments)
  • data/MCViNE/ – small test inputs (toy fcc phonon IDF set and atoms file, Debye DOS, S(q,w) grid, SANS map; ~0.6 MB)

Where the port differs from MCViNE (documented in contrib/doc/MCViNE\_kernels.md):

  • Absorbed events: kinematically forbidden events are absorbed; MCViNE passes them on unchanged.
  • Grids: grid data is interpolated rather than looked up bin by bin.
  • Two bug fixes: mcvine/acc's SANS2D velocity update, and the PolyXtal "two E_f choices" factor.
  • unbiased=1 option: available on four kernels whose MCViNE sampling loops are slightly biased; the default reproduces MCViNE.

Tests

  • Test instruments: each Test\_MCViNE\_<kernel> instrument runs the same sample as the Union process (comp\_select=1) or as the standalone component (comp\_select=2), with the same geometry, absorption and multiple scattering. It has an %Example line for each form.
  • mctest results: all 32 %Example cases pass in mctest --no-mpi at 100 % of their recorded values. The Union and standalone values of each kernel agree within about 2σ (0.3–1 % for most kernels; SQE, SANS2D and SingleXtal have larger statistical errors).
Test Union Standalone ratio
Test_MCViNE_Broadened_E_Q 5.698e-07 5.68e-07 1.003 ± 0.004
Test_MCViNE_ConstantEnergyTransfer 6.369e-07 6.362e-07 1.001 ± 0.003
Test_MCViNE_ConstantQE 6.362e-07 6.383e-07 0.997 ± 0.003
Test_MCViNE_ConstantvQE 3.172e-07 3.157e-07 1.005 ± 0.003
Test_MCViNE_DGSSXRes 6.476e-13 6.503e-13 0.996 ± 0.003
Test_MCViNE_E_Q 5.519e-07 5.529e-07 0.998 ± 0.004
Test_MCViNE_E_vQ 1.77e-07 1.788e-07 0.990 ± 0.008
Test_MCViNE_LorentzianBroadened_E_Q 5.71e-07 5.691e-07 1.003 ± 0.004
Test_MCViNE_Phonon_CoherentInelastic_PolyXtal 1.273e-07 1.266e-07 1.005 ± 0.017
Test_MCViNE_Phonon_CoherentInelastic_SingleXtal 6.618e-08 6.531e-08 1.013 ± 0.020
Test_MCViNE_Phonon_IncoherentElastic 9.022e-07 9.005e-07 1.002 ± 0.003
Test_MCViNE_Phonon_IncoherentInelastic 1.457e-07 1.452e-07 1.003 ± 0.004
Test_MCViNE_SANS2D_ongrid 9.722e-07 9.425e-07 1.032 ± 0.021
Test_MCViNE_SQE 2.433e-06 2.247e-06 1.083 ± 0.049
Test_MCViNE_SQ_elastic 6.366e-07 6.38e-07 0.998 ± 0.004
Test_MCViNE_SvQ 3.443e-07 3.433e-07 1.003 ± 0.003
  • Physics validation (scripts attached): analytic integrals, McStas Incoherent, and independent Python calculations. Examples:

    • S(Q)=Q² gives 57.89 ± 0.17 vs 2k² = 57.91.
    • The Debye–Waller core from the DOS is 0.004309 Ų, vs 0.004316 Ų by independent quadrature.
    • One-phonon incoherent intensity is 0.1543 vs 0.1544 by quadrature.
    • Single-crystal coherent phonons give 0.214 vs 0.207 from an independent calculation with exact eigenvectors.
    • Transport matches Incoherent within 0.3 % for single scattering, and an analog MC within 0.2 % for double scattering.
  • Documentation: mcdoc renders the parameter tables of all 32 components.

  • Formatting: mccode-clangformat has been applied.

  • Linting: the shared library is clean under gcc -std=c99 -Wall -Wextra and cppcheck --enable=warning,portability,performance.

---

Declaration of use of AI-tools

  • Please add a checkmark here if you used AI-tools during the work for this contribution

  • Furter, please describe how / where and for what the tools were used:

    Claude (Anthropic, Claude Code) was used. It compared the MCViNE and McStas code bases to find the missing kernels, ported the MCViNE C++ kernels to C (share/mcvine-lib.c), and wrote the Union glue and the 12-line Union core registration.
    I reviewed the ported physics against the MCViNE sources and the validation results.

---

Development OS / boundary conditions

Developed and tested on Linux (Ubuntu 24.04, gcc 13, x86_64) with McStas built from main @ d3ab8cf, using mcrun/mctest from the same build without MPI. No extra dependencies are needed. Two test instruments are slow at the default 1e6 neutrons in Union mode: Test\_MCViNE\_Phonon\_CoherentInelastic\_SingleXtal (about 150 s) and Test\_MCViNE\_E\_vQ (about 90 s).

---

PR Checklist for contributing to McStas/McXtrace

  • My contribution includes a new component file

    • I have ensured that naming of parameters are in the style of existing components (NOMENCLATURE: geometry, sigma\_\*, Vc, p\_transmit, target\_index/focus\_r, Union packing\_factor/interact\_fraction)
    • I have ensured that component parameters are in the usual units of McStas (m, meV, AA^-1, barn, AA^3, K)
    • I have used the mcdoc utility and rendered a reasonable documentation page for the component (screenshots attached)
    • I have ensured that basic use of the component is OK (all 32 compile and run in the test instruments)
    • I have included a corresponding example instrument and will fill in the new instrument section below
    • I have used the mccode-clangformat tool to apply the standard McCode component indentation scheme
    • My new component is added within the contrib component category
  • My contribution includes a new instrument file

    • I have used the mcdoc utility and rendered a reasonable documentation page for the instrument
    • I have ensured that basic use of the instrument is OK (e.g. it compiles?)
    • ... and provided reasonable default parameters in that instrument that produce reasonable output
    • ... and maybe even added a %Example: line to describe expected behaviour (two per instrument)
    • I have used the mcrun --c-lint "linter" and followed advice to remove most / all warnings that are raised (the new library code was linted with cppcheck and is clean)
    • My new instrument is added within the examples hierarchy (examples/Tests\_samples/Test\_MCViNE\_\*/)
    • My new instrument has a new, unique filename, not clashing with existing example instruments
    • My new instrument requires a data/input file. The shared test inputs are in the global data/MCViNE/ folder.
  • My work touches / adds to the runtime lib code (.c,.h etc in multiple locations

    • I have added reasoning and documentation for the change below: see "Runtime-library change" above; the Union core only gains the MCViNE enum entry, union member and two dispatch cases
    • I am attaching test output in the comments

🤖 Generated with Claude Code

Fahima-Islam and others added 4 commits September 30, 2026 04:33
Add the MCViNE entry to enum process, a pointer_to_a_MCViNE_physics_storage_struct
member to union data_transfer_union, and the two dispatch cases in physics_my and
physics_scattering (guarded by PROCESS_MCVINE_DETECTOR). All MCViNE kernel
processes share this one type.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YUUwywmxwaAvj7JoVQJ9y2
mcvine-lib: ports of the MCViNE sample kernels (mccomponents/lib/kernels/sample
and mcvine/acc SANS2D_ongrid), a run-time expression evaluator, table and grid
readers, DOS / Debye-Waller helpers, MCViNE IDF phonon readers, sample shapes and
the MCViNE HomogeneousNeutronScatterer transport.
mcvine-union-lib: glue between the kernels and the Union framework.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YUUwywmxwaAvj7JoVQJ9y2
MCViNE_<kernel>_process (Union) and MCViNE_<kernel> (standalone) for
ConstantEnergyTransfer, ConstantQE, ConstantvQE, E_Q, Broadened_E_Q,
LorentzianBroadened_E_Q, E_vQ, SQ, SvQ, SQE, DGSSXRes, SANS2D_ongrid,
Phonon_IncoherentElastic, Phonon_IncoherentInelastic,
Phonon_CoherentInelastic_PolyXtal and Phonon_CoherentInelastic_SingleXtal.
Documentation in contrib/doc/MCViNE_kernels.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YUUwywmxwaAvj7JoVQJ9y2
One Tests_samples/Test_MCViNE_<kernel> instrument per kernel, running the Union
process (comp_select=1) or the standalone component (comp_select=2) on the same
sample, with an %Example line for each. Test inputs in data/MCViNE.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YUUwywmxwaAvj7JoVQJ9y2
@Fahima-Islam

Copy link
Copy Markdown
Contributor Author
examples_overview mcdoc_MCViNE_E_Q mcdoc_MCViNE_Phonon_CoherentInelastic_SingleXtal_process mcdoc_Test_MCViNE_E_Q [mctest_output.txt](https://github.com/user-attachments/files/32847866/mctest_output.txt) mcviewtest_report union_examples_overview

@willend

willend commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

@Fahima-Islam this looks absolutely great, good job! :-)
I will thus archive #2686

The compilation issue for mcstas-antlr is intermittent and will be fixed via an incoming PR.

@mads-bertelsen

Copy link
Copy Markdown
Contributor

Wow, great job! Really like how the code is organized such that duplication is avoided across the stand alone samples and Union processes. This offers a lot of exciting new capabilities! The run times are also faster than I feared for the advanced inelastic capabilities.

You mention that duplicated efforts such as 2D grid (Q,E) are not ported, and of course its extra work to do that, but in general we welcome having several independent ways to calculate the same thing for a good cross check.

@willend

willend commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

@Fahima-Islam absolutely nice stuff indeed. :-)

Just a word of warning - the Windows platform will require a bit more work it seems, but Claude might help you with that. (Pick the right artefact from https://github.com/mccode-dev/McCode/actions/runs/36691645550?pr=2716 and investigate the failing new MCVINE instrument compile_stdout.txt.)

Most often the issues are along the lines of

  • gnu style double blah[var]; allocations need replacement by malloc()'s
  • certain POSIX assumptions may need local plugs / workarounds
  • if anything is typed complex double you need to rephrase vars / expressions to use mccode-complex-lib

Screenshot of Windows state:
Screenshot 2026-09-30 at 13 17 15

@willend

willend commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

@Fahima-Islam I have dug out the Windows results for you
MCViNE_failures.tgz

It looks like a number of them indeed are things like replacements of double e1[3], e2[3], e3[3], d[3]; -> malloc's.

I will boot up my Windows later to see if I can get a hold of some of this...

rpcndr.h (pulled in by windows.h) has '#define small char', so
'int i, small;' in mcvine_S_Phonon_IncoherentInelastic became
'int i, char;' under MSVC and broke the build of every instrument
that includes mcvine-lib.c.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Fahima-Islam

Copy link
Copy Markdown
Contributor Author

@willend thanks a lot for digging out the Windows results!

All 16 Test_MCViNE_* failures turned out to have a single cause: a local variable named small in mcvine_S_Phonon_IncoherentInelastic (share/mcvine-lib.c). rpcndr.h, pulled in by windows.h, has #define small char, so under MSVC int i, small; became int i, char;. That gives exactly the C2059 / C2513 / C2181 errors in the logs, and since mcvine-lib.c goes into every MCViNE instrument, all of them failed.

5f6df3c renames it to is_small (3 lines). I checked it with the Test_MCViNE_E_Q.c generated on the Windows runner, compiled with -Dsmall=char: the original fails at the same three lines, the fixed one compiles. I haven't found other identifiers in the PR that clash with Windows macros. The fixed-size arrays (double e1[3] etc.) are fine for MSVC. The min/max C4005 warnings come from the monitor's parameters in the test instrument, not from the MCViNE code.

Let's see what the Windows CI says now.

@willend

willend commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

@Fahima-Islam great!

@willend

willend commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Seems to do he trick indeed @Fahima-Islam - snipped from the Windows compile log:

Test_MCViNE_Broadened_E_Q                       :   2.83
Test_MCViNE_ConstantEnergyTransfer              :   2.58
Test_MCViNE_ConstantQE                          :   2.58
Test_MCViNE_ConstantvQE                         :   2.69
Test_MCViNE_DGSSXRes                            :   2.62
Test_MCViNE_E_Q                                 :   2.70
Test_MCViNE_E_vQ                                :   2.71
Test_MCViNE_LorentzianBroadened_E_Q             :   2.51
Test_MCViNE_Phonon_CoherentInelastic_PolyXtal   :   2.61
Test_MCViNE_Phonon_CoherentInelastic_SingleXtal :   2.64
Test_MCViNE_Phonon_IncoherentElastic            :   2.62
Test_MCViNE_Phonon_IncoherentInelastic          :   2.64
Test_MCViNE_SANS2D_ongrid                       :   2.64
Test_MCViNE_SQE                                 :   2.67
Test_MCViNE_SQ_elastic                          :   2.51
Test_MCViNE_SvQ                                 :   2.69

FYI we are also in the midst of restructuring parts of Union, among other things leading to a solution where Union_init and Union_stop are dropped / removed. We may decide finalise that process between @mads-bertelsen, @g5t and I before we merge this PR. My hopes are still that we will make that and v3.8.9 in maximum a weeks time. :-)

@Fahima-Islam

Copy link
Copy Markdown
Contributor Author

@willend thanks for confirming! All CI has now finished:

  • Windows (McStas and McXtrace) passes; all 16 MCViNE instruments compile and run.
  • All 32 MCViNE tests (Union process and standalone for each kernel) pass on every platform, with values at 97–100 % of the %Example references.
  • ubuntu-latest.conda.antlr fails with the same single compile error as main, which you mentioned is being fixed separately.
  • ubuntu-24.04.source.classic.gcc-13 (mcstas-basictest) fails with runtime errors in ILL_H5 and ILL_H5_new. They don't use MCViNE or Union, and both passed in this PR's first CI round. Between the two rounds main gained PowderN, Single_crystal: Support multiphase and single-crystal NCrystal materials #2715 (PowderN / NCrystal multiphase) and NCrystal: Search for data files in the directory of the instrument file (localdata/) #2714 (Open_File search paths), and ILL_H5 uses PowderN and reads several data files. Since main only tests changed instruments, this may be a regression on main that only shows up in PRs that trigger the full test set. Could you take a look?

On the Union restructuring: no problem with waiting. Once Union_init / Union_stop are gone I'm happy to adapt the _process components and the test instruments to the new setup, so just ping me when it has landed.

@Fahima-Islam

Copy link
Copy Markdown
Contributor Author

@mads-bertelsen thank you! I'm glad the code structure and the run times work for you.

Good point about independent implementations as cross-checks. I left those kernels out to keep this PR focused, but they would be straightforward to add in a separate follow-up PR using the same mcvine-lib / MCViNE process-type setup, so no further Union core changes would be needed. Candidates are MCViNE's isotropic, gridded S(Q,E), powder and single-crystal diffraction, SANS sphere and multiphonon kernels, each with test instruments comparing against the existing McStas components (Incoherent, Isotropic_Sqw, PowderN, Single_crystal, the SANS samples and IncoherentPhonon_process). MCViNE's EPSC diffraction kernel is unfinished upstream, so I'd leave it out.

This kind of work has also become much faster with AI-assisted ("vibe") coding: this PR, porting 16 kernels from MCViNE's C++, the Union glue and the test instruments, came together far quicker than it would have by hand. Most of my time went into checking the physics and validating against MCViNE and the references. So a follow-up with the overlapping kernels is a realistic next step rather than a long project, and having two independent implementations side by side is exactly the kind of cross-check that makes the fast route trustworthy.

I'd start that after this PR is merged and the Union restructuring has settled, so it is built on the new Union interface. Does that sound good?

@willend

willend commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

@Fahima-Islam I've recently seen intermittent failures on
ubuntu-24.04.source.classic.gcc-13 (mcstas-basictest) in ILL_H5 and ILL_H5_new (I believe on a scale of ~ a few weeks) so I am tempted to conclude these are unrelated to both of your changes (of course :-) ) and all of the other stuff that just came in... :-)

As a side-note both of ILL_H5 and ILL_H5_new are "full guide-hall simulations" that includes a plethora of JUMPs in combination with EXTEND, WHEN, GROUP, COPY SPLIT etc... So in this way an anomaly compared to the overall examples/.

I think I will simply add an issue for that to later perform a separate investigation.

@willend

willend commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

@Fahima-Islam #2712 is merged to main. I have instructed my local Claude to update this PR with the required changes.

Main now has the self-contained Union library (mccode-dev#2712): the Union types
moved from union-lib.c into union-lib.h, and union-suffix.c is gone,
since each process registers its own dispatch case.

Conflicts resolved:
- union-lib.c: keep main's version. The MCViNE entry in enum process and
  the pointer_to_a_MCViNE_physics_storage_struct member of
  data_transfer_union move to union-lib.h, where those types now live.
- union-suffix.c: deleted, as on main. The MCViNE dispatch cases it held
  are registered by the MCViNE components in the following commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QQzBnsrndv9v3cLryvVaR
@willend

willend commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Context from #2712 re-used and further prompted:

All now good and merged to main. Please inspect #2716 and consequence-edit on the new union-components and related instruments.

mccode-dev#2712 made the Union components self-contained, so the MCViNE processes
no longer rely on the code generator to load the Union library or on
union-suffix.c for their dispatch:

- Each MCViNE_*_process loads the library with %include "union-lib"
  instead of guarding on #ifndef Union, takes the shared state with
  union_acquire() in INITIALIZE, and gives it back with union_release()
  in a new FINALLY section.
- mcvine_union_register() takes the process list to add to, and the
  processes pass &union_state_p->u_process_list instead of the removed
  g_process_list global.
- union-lib.h gets empty default dispatch macros for MCViNE, and
  mcvine-union-lib.h, which every MCViNE process includes, replaces them
  with the two case MCViNE: entries. PROCESS_MCVINE_DETECTOR, which only
  guarded the union-suffix.c cases, is dropped.
- Like the other Union components, the processes accept the deprecated,
  unused string init="" parameter.
- The component headers, mcvine-union-lib.h and MCViNE_kernels.md
  describe the new registration instead of union-lib.c/union-suffix.c.

The 16 Test_MCViNE_* instruments need no change. Run with comp_select=1
and 2 at a fixed seed, all 32 detector outputs are identical to the PR as
submitted (classic mcstas with the former code generator injection), both
with the current classic mcstas and with mccode-antlr 0.30.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QQzBnsrndv9v3cLryvVaR
@willend

willend commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

I've adapted PR #2716 to the self-contained Union scheme now on main. It's a new local branch mcvine-kernels in your checkout, two commits on top of the PR head (5f6df3c). Nothing is pushed and your main checkout is untouched.

835c736 Merge origin/main into mcvine-kernels. This resolves the two conflicts with the merged #2712:

union-lib.c: I kept main's version. The PR's MCViNE entry in enum process and its storage-pointer member move to union-lib.h, where those types now live.
union-suffix.c: deleted, as on main.

5e2720f Adapt the MCViNE Union processes to the self-contained Union library

16 MCViNE_process.comp:
Each loads the library with %include "union-lib" instead of the #ifndef Union guard.
Each calls union_acquire() in INITIALIZE and union_release() in a new FINALLY section.
Each accepts the deprecated, unused string init="" parameter.
The header text no longer points at union-suffix.c.
mcvine-union-lib.c/.h: mcvine_union_register() now takes the process list as an argument instead of using the removed g_process_list global. The two case MCViNE: dispatch entries are registered once, in mcvine-union-lib.h, which every MCViNE process includes. union-lib.h gets the matching empty defaults, and PROCESS_MCVINE_DETECTOR is dropped.
MCViNE_kernels.md: the registration description is updated.
The 16 Test_MCViNE
instruments: no change needed. None of them uses Union_init.

Testing: I ran all 16 test instruments with comp_select=1 and comp_select=2 at a fixed seed. All 32 detector outputs are identical to the PR as submitted on the old main, both with classic mcstas and with mccode-antlr 0.30.0. The original PR would not have built with mccode-antlr.

To update the PR: the branch fast-forwards from the contributor's head, so no force-push is needed. Assuming the fork is named McCode, that's git push https://github.com/Fahima-Islam/McCode.git mcvine-kernels:mcvine-kernels. This only works if "Allow edits by maintainers" is enabled on the PR, which I couldn't check from here. Otherwise, hand the branch to Fahima.

@willend

willend commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

antlr-run fails with a single unrelated runtime-issue
Test_NeXus_comp_par_runtime : RUNTIME ERROR, mcrun -s 1000 Test_NeXus_comp_par_runtime -y --format=NeXus -n1e6 --mpi=auto -d1 > run_stdout_1.txt 2>&1

Will investigate separately, likely fixed via incoming #2729

If this ends up as the only issue / error I plan to merge @Fahima-Islam. Changes will make it to v3.8.9.

@willend

willend commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

All tests except antlr-based came back without error, merging.

(Investigating the unrelated antlr-compile-error separately but suspect that #2729 will correct.)

@willend
willend merged commit c4bbf56 into mccode-dev:main Oct 2, 2026
12 of 13 checks passed
@mads-bertelsen

Copy link
Copy Markdown
Contributor

@mads-bertelsen thank you! I'm glad the code structure and the run times work for you.

Good point about independent implementations as cross-checks. I left those kernels out to keep this PR focused, but they would be straightforward to add in a separate follow-up PR using the same mcvine-lib / MCViNE process-type setup, so no further Union core changes would be needed. Candidates are MCViNE's isotropic, gridded S(Q,E), powder and single-crystal diffraction, SANS sphere and multiphonon kernels, each with test instruments comparing against the existing McStas components (Incoherent, Isotropic_Sqw, PowderN, Single_crystal, the SANS samples and IncoherentPhonon_process). MCViNE's EPSC diffraction kernel is unfinished upstream, so I'd leave it out.

This kind of work has also become much faster with AI-assisted ("vibe") coding: this PR, porting 16 kernels from MCViNE's C++, the Union glue and the test instruments, came together far quicker than it would have by hand. Most of my time went into checking the physics and validating against MCViNE and the references. So a follow-up with the overlapping kernels is a realistic next step rather than a long project, and having two independent implementations side by side is exactly the kind of cross-check that makes the fast route trustworthy.

I'd start that after this PR is merged and the Union restructuring has settled, so it is built on the new Union interface. Does that sound good?

@Fahima-Islam that sounds great, and the work done to validate is great, that makes it a far more professional contribution than what “vibe coding” can sound like.

@Fahima-Islam

Copy link
Copy Markdown
Contributor Author

@willend @mads-bertelsen thank you both!

@willend thanks a lot for adapting the PR to the new self-contained Union scheme yourself, and for checking that all 32 test outputs stayed identical, with mccode-antlr as well. That saved me a round trip, and I'm glad the MCViNE kernels will make it into v3.8.9. Thanks also for taking ILL_H5 to a separate issue.

@mads-bertelsen thank you for the kind words about the validation. That was where most of the effort went, so it's great to hear it came across. I'd like to start the follow-up PR with the overlapping kernels (isotropic, gridded S(Q,E), powder, single-crystal, SANS spheres and multiphonon), built on the new Union interface from current main, with test instruments comparing each kernel against its existing McStas counterpart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants